Add support for gtp with multiple traffic policies #162
Conversation
Signed-off-by: aattuluri <anil_attuluri@intuit.com>
Signed-off-by: aattuluri <anil_attuluri@intuit.com>
Signed-off-by: aattuluri <anil_attuluri@intuit.com>
Signed-off-by: aattuluri <anil_attuluri@intuit.com>
This reverts commit a9fa409.
…mmit sha if neither i) or ii)
.circleci/config.yml
Outdated
| @@ -90,18 +90,18 @@ jobs: | |||
| cd tests | |||
| export IS_LOCAL=false | |||
| ./run.sh "1.16.8" "1.5.7" "../out" | |||
| # - run: | |||
There was a problem hiding this comment.
Let's not keep commented out code
admiral/pkg/clusters/handler.go
Outdated
| if gtpWrapper != nil { | ||
| processGtp := true | ||
| if len(locality) == 0 { | ||
| log.Errorf(LogErrFormat, "Process", "GlobalTrafficPolicy", host, "", "Skipping gtp processing, locality of the cluster nodes cannot be determined. Is this minikube?") |
There was a problem hiding this comment.
Should this be warning? Since it seems like the overall processing is able to continue
admiral/pkg/clusters/serviceentry.go
Outdated
| func AddServiceEntriesWithDr(cache *AdmiralCache, sourceClusters map[string]string, rcs map[string]*RemoteController, serviceEntries map[string]*networking.ServiceEntry) { | ||
| syncNamespace := common.GetSyncNamespace() | ||
| for _, se := range serviceEntries { | ||
|
|
||
| //add service entry | ||
| serviceEntryName := getIstioResourceName(se.Hosts[0], "-se") | ||
| //Add a label |
There was a problem hiding this comment.
Is this comment accurate?
admiral/pkg/clusters/serviceentry.go
Outdated
| var modifiedSe *networking.ServiceEntry | ||
| var host = se.Hosts[0] | ||
| var drName, seName = defaultDrName, defaultSeName | ||
| if gtpTrafficPolicy.Dns != env && gtpTrafficPolicy.Dns != common.Default { |
There was a problem hiding this comment.
Do you possible mean some other comparison here? Is it possible that the DNS = env?
install/sample/gtp.yaml
Outdated
| labels: | ||
| identity: greeting | ||
| spec: | ||
| policy: | ||
| - dns: stage.greeting.global | ||
| - dns: default # default is a keyword, alternatively you can use `env` (ex: stage) |
There was a problem hiding this comment.
Connecting to my previous comment Do you possible mean some other comparison here? Is it possible that the DNS = env? - could we maybe document this behavior a little bit more clearly? I find it a little counterintuitive that the DNS field should be the relevant environment (and it's distinctly different from how the docs say things work: https://github.com/istio-ecosystem/admiral/blob/master/docs/Architecture.md)
|
The change from the DNS field in the GTP to be the environment seems to be backwards incompatible to me. Are we concerned that may break any existing users? |
I changed the implementation to treat dns as a dns prefix rather than FQQN, the rationale is as follows: That being said, I can add an additional check to see if the dns in gtp matches the full default host for backward compatibility but that will retain hybrid behavior (dns prefix vs full dns), so I think it makes sense to introduce this backward incompatible change and then call it out in the release notes. |
|
I agree 100% that this is a good direction in which to go. I just want to make sure we're not breaking anyone else. Not sure how careeful we want to be, but the most foolproof option would probably be to introduce a new field in the GTP spec (something like |
There was a problem hiding this comment.
Approving as all of your updates look good, but please update the GTP section here: https://github.com/istio-ecosystem/admiral/blob/master/docs/Architecture.md to use the DNS prefix and show how it's used.
* Add support for gtp with multiple traffic policies (#162) Fixes #163 Fixes #161 Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Fix mesh port match against to look at k8s svc target port (#165) Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Add basic health check API (#171) * some basic folder * get the basic curl /health/ready working * Refactoring api server files * reverting a very tiny comment that I shouldn't have pushed Signed-off-by: Mengying <mengyinglimandy@gmail.com> * trying to fix the circle CI error Signed-off-by: Mengying <mengyinglimandy@gmail.com> * addressed the comment Signed-off-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Admiral apis (#175) * added the get all cluste api Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * fixed the unit test Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * updated the comment a little bit Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Adding api to get service entries based on given cluster or given identity Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Refactoring api code Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing failing tests for service.go Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments and ci failures Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * added some unit tests Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added the cluster id inside deployment controller (#176) * added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added unit test for api function GetServiceEntriesByCluster Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed a small typing in test Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed some indentation Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added test for get se by identity Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id to be print in log Co-authored-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: Mengying-Li <43981707+Mengying-Li@users.noreply.github.com> * added more context to the API call Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: vrushalijoshi <vrushalijoshi.cummins@gmail.com>
* Add support for gtp with multiple traffic policies (istio-ecosystem#162) Fixes istio-ecosystem#163 Fixes istio-ecosystem#161 Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Fix mesh port match against to look at k8s svc target port (istio-ecosystem#165) Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Add basic health check API (istio-ecosystem#171) * some basic folder * get the basic curl /health/ready working * Refactoring api server files * reverting a very tiny comment that I shouldn't have pushed Signed-off-by: Mengying <mengyinglimandy@gmail.com> * trying to fix the circle CI error Signed-off-by: Mengying <mengyinglimandy@gmail.com> * addressed the comment Signed-off-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Signed-off-by: Mengying <mengyinglimandy@gmail.com> * Admiral apis (istio-ecosystem#175) * added the get all cluste api Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * fixed the unit test Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * updated the comment a little bit Signed-off-by: Mengying <mengyinglimandy@gmail.com> Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Adding api to get service entries based on given cluster or given identity Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Refactoring api code Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing failing tests for service.go Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments and ci failures Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * Fixing review comments Signed-off-by: vjoshi3 <vrushali_joshi@intuit.com> * added some unit tests Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added the cluster id inside deployment controller (istio-ecosystem#176) * added the cluster id inside deployment controller Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed the helm setup failure Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id in other remote controllers Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added unit test for api function GetServiceEntriesByCluster Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed a small typing in test Signed-off-by: Mengying <mengyinglimandy@gmail.com> * fixed some indentation Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added test for get se by identity Signed-off-by: Mengying <mengyinglimandy@gmail.com> * added cluster id to be print in log Co-authored-by: Mengying <mengyinglimandy@gmail.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: Mengying-Li <43981707+Mengying-Li@users.noreply.github.com> * added more context to the API call Co-authored-by: aattuluri <44482891+aattuluri@users.noreply.github.com> Co-authored-by: vjoshi3 <vrushali_joshi@intuit.com> Co-authored-by: vrushalijoshi <vrushalijoshi.cummins@gmail.com> Signed-off-by: psikka1 <pankaj_sikka@intuit.com>
No description provided.